8266666: Implementation for snippets #4795

pavelrappo · 2021-07-15T14:13:16Z

This PR implements JEP 413 "Code Snippets in Java API Documentation", which hasn't been yet proposed to target JDK 18. The PR starts as a squashed merge of the https://github.com/openjdk/jdk-sandbox/tree/jdk.javadoc/snippets branch.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8266666: Implementation for snippets

Reviewers

Jonathan Gibbons (@jonathan-gibbons - Reviewer) ⚠️ Review applies to edb7cf8

Contributors

Jonathan Gibbons <jjg@openjdk.org>
Hannes Wallnöfer <hannesw@openjdk.org>

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/4795/head:pull/4795
$ git checkout pull/4795

Update a local copy of the PR:
$ git checkout pull/4795
$ git pull https://git.openjdk.java.net/jdk pull/4795/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4795

View PR using the GUI difftool:
$ git pr show -t 4795

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/4795.diff

This undoes use of the feature in JDK, but leaves the implementation.

bridgekeeper · 2021-07-15T14:13:41Z

👋 Welcome back prappo! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

pavelrappo · 2021-07-15T14:16:21Z

/contributor add jjg
/contributor add hannesw

openjdk · 2021-07-15T14:16:39Z

@pavelrappo
Contributor Jonathan Gibbons <jjg@openjdk.org> successfully added.

pavelrappo · 2021-07-15T14:17:20Z

/contributor add hannesw

openjdk · 2021-07-15T14:17:45Z

@pavelrappo
Contributor Hannes Wallnöfer <hannesw@openjdk.org> successfully added.

openjdk · 2021-07-15T14:18:28Z

@pavelrappo
Contributor Hannes Wallnöfer <hannesw@openjdk.org> successfully added.

openjdk · 2021-07-15T14:19:58Z

@pavelrappo The following labels will be automatically applied to this pull request:

compiler
javadoc

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

mlbridge · 2021-07-15T14:24:05Z

Webrevs

jonathan-gibbons · 2021-07-23T17:04:44Z

/csr

openjdk · 2021-07-23T17:05:47Z

@jonathan-gibbons this pull request will not be integrated until the CSR request JDK-8266669 for issue JDK-8266666 has been approved.

jonathan-gibbons

The overall structure looks generally good. Nice work.

That being said, I note the following ...

All new top-level classes should have a reasonable documention comment, including where appropriate the standard boilerplate (i.e. not on public API classes and interfaces)

* <p><b>This is NOT part of any supported API.
* If you write code that depends on this, you do so at your own risk.
* This code and its internal interfaces are subject to change or
* deletion without notice.
All new non-private methods should have a reasonable documentation comment
There's more than a typical number of chatty comments and/or fixme comments and/or code that needs to be uncommented. I'll do a separate review pass to note some of those examples.
Please work with @hns to determine the proposed CSS style(s) to be used, and not just the interim placeholder sandybrown. The color should be compatible with the existing color palette. Ideally, at the same time, in either this changeset or a sibling changeset, we should change the style of a default <pre>{code} block to be similar with respect to the basic display characteristics.

jonathan-gibbons

More feedback;

It seems reasonable to introduce the ...taglets.snippets package. It seems excessive to have three sub packages within that, for actions, parser and text, especially when the last contains just a single class.
At some point soonish, we will need API somewhere to get the content of the snippet. This is necessary to fulfill the goal in the JEP to support validation of snippet content. Without any such API, validation code would have to duplicate at least some of the code to handle snippets. This API should probably not be in com.sun.source and should probably be in the jdk.javadoc module, perhaps on the existing StandardDoclet class. One way to do that would be to introduce a utility interface to provide access to support for taglets, and then have a method on StandardDoclet to access an instance of that utility for a specific snippet, identified by element and/or id.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/HtmlDocletWriter.java

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/resources/stylesheet.css

pavelrappo · 2021-07-26T12:06:06Z

All new top-level classes should have a reasonable documention comment, including where appropriate the standard boilerplate (i.e. not on public API classes and interfaces)

This is NOT part of any supported API.

If you write code that depends on this, you do so at your own risk.

This code and its internal interfaces are subject to change or

deletion without notice.

All new non-private methods should have a reasonable documentation comment

I'm ok with adding the boilerplate "This is NOT part of any supported API" note, as well as adding method comments where practical. However, I note that this PR comprises mostly an internal implementation and not a public API. Internal implementation comments are more like documentation rather than specification. Such documentation is virtually never built and is read in an IDE while eyeballing the respective code and tests. I think it's overkill and waste of resources to use doc comments in such a case.

There's more than a typical number of chatty comments and/or fixme comments and/or code that needs to be uncommented. I'll do a separate review pass to note some of those examples.

I'm currently working on removing as many FIXMEs and TODOs as possible. I think of a FIXME comment as a "review breakpoint" that highlights an issue that has to be resolved before the code is accepted.

I would be interested to learn which comments you consider "chatty". I look forward to fixing them with your help. One note, though. For an internal implementation, I try hard to make only the "why" comments, not the "what" comments. "What" comments have their place, but they should be sparing and reserved for non-trivial domains. For trivial domains, the "what" should be obvious. If it is not, I failed at coding, not at commenting.

pavelrappo · 2021-07-26T12:55:37Z

More feedback;

It seems reasonable to introduce the ...taglets.snippets package. It seems excessive to have three sub packages within that, for actions, parser and text, especially when the last contains just a single class.

That package structure was initially there to keep the dependencies clear. That said, I'm okay with squashing it into a single package for practical reasons.

At some point soonish, we will need API somewhere to get the content of the snippet. This is necessary to fulfill the goal in the JEP to support validation of snippet content. Without any such API, validation code would have to duplicate at least some of the code to handle snippets. This API should probably not be in com.sun.source and should probably be in the jdk.javadoc module, perhaps on the existing StandardDoclet class. One way to do that would be to introduce a utility interface to provide access to support for taglets, and then have a method on StandardDoclet to access an instance of that utility for a specific snippet, identified by element and/or id.

We can discuss that API either in this PR or in a separate thread on javadoc-dev at openjdk.java.net. (Separately: is this @/at trick still efficient? Modern email-address harvesting is surely smarter than that.)

mlbridge · 2021-07-26T16:31:57Z

Mailing list message from Jonathan Gibbons on javadoc-dev:

The comments don't need to be of core-libs quality, but at least some of
the problems we have had in the code of late has been discerning the
original intent, so at least some amount of comment/documentation is
useful, even if we don't run it through javadoc or even doclint.

Think of the comments as "pay it forward" to our future selves.

-- Jon

On 7/26/21 5:09 AM, Pavel Rappo wrote:

I'm ok with adding the boilerplate "This is NOT part of any supported API" note, as well as adding method comments where practical. However, I note that this PR comprises mostly an internal implementation and not a public API. Internal implementation comments are more like documentation rather than specification. Such documentation is virtually never built and is read in an IDE while eyeballing the respective code and tests. I think it's overkill and waste of resources to use doc comments in such a case.

hns

Overall the code looks good!

A few comments inline. I'll have a look at the stylesheet changes next.

src/jdk.compiler/share/classes/com/sun/source/doctree/SnippetTree.java

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/SnippetTaglet.java

...c/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/snippet/text/AnnotatedText.java

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/resources/stylesheet.css

...avadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/snippet/parser/Parser.java

This commit adds trivial commentary to some of the Java declarations. This commit also adds the "This is NOT part of any supported API..." disclaimer.

pavelrappo · 2021-07-27T15:37:50Z

Mailing list message from Jonathan Gibbons on javadoc-dev:

The comments don't need to be of core-libs quality, but at least some of
the problems we have had in the code of late has been discerning the
original intent, so at least some amount of comment/documentation is
useful, even if we don't run it through javadoc or even doclint.

Think of the comments as "pay it forward" to our future selves.

-- Jon

On 7/26/21 5:09 AM, Pavel Rappo wrote:

I'm ok with adding the boilerplate "This is NOT part of any supported API" note, as well as adding method comments where practical. However, I note that this PR comprises mostly an internal implementation and not a public API. Internal implementation comments are more like documentation rather than specification. Such documentation is virtually never built and is read in an IDE while eyeballing the respective code and tests. I think it's overkill and waste of resources to use doc comments in such a case.

Project conventions are more important than my preferences. Although I don't see much value in such comments, I added some in commit 4300903.

This commit moves the contents of the jdk.javadoc.internal.doclets.toolkit.taglets.snippet.{action,parser,text} packages into the jdk.javadoc.internal.doclets.toolkit.taglets.snippet package.

DocCommentParser now recognizes end-of-input better. To test that in testBadTagSyntax, the ToolBox.writeJavaFiles approach was used instead that of ClassBuilder.write. This is because ClassBuilder muddled doc comments with extra formatting such as trailing newline. So from within `ClassBuilder.MethodBuilder.setComments(java.lang.String...)` one couldn't make an immediate end-of-input: there was always `\n` appended to the comment. Also reformatted testBadTagSyntax for ease of comparing input and expected error output.

Improves code style; reformats.

Removes stale FIXMEs, TODOs and comments; downgrades FIXMEs to TODOs where possible; wraps overly long lines.

hns

I went through the changes again, they mostly look good. There are a few areas (mostly handling of tags and styled text) where I still want to do more exploring in order to better understand the code.

I left inline comments for minor issues/questions I encountered.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/HtmlDocletWriter.java

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/CommentUtils.java

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/snippet/Parser.java

jonathan-gibbons

The primary actionable item is that all strings that are intended for use in end-user error messages need to be localizable. i.e. see Parser.ParseException

Other comments are minor and can be deferred.

src/jdk.compiler/share/classes/com/sun/source/util/SimpleDocTreeVisitor.java

src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/TagletWriterImpl.java

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/snippet/Parser.java

...vadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/snippet/ParseException.java

1. Reverts changes from CommentUtils.java (unused methods) 2. Changes misguided regex-related assertion to tests

* Cleans up error string format * Internationalizes error messages, mapping them to fewer resource keys * Fixes a few bugs related to error messages, including: inaccurate caret positioning and runaway resource name (i.e. the resource key wasn't translated into an error)

jonathan-gibbons

My primary concern in the last round of review was internationalization. You've addressed that: good.

When reading, and re-reading, a contribution this big, it's always possible to see new tiny details that could be improved. I've mentioned some here, and I'm sure that in times to come we will look at the code more and maybe see more tweaks. So, enough for now. Well done.

I note the varied references to improve JavadocTester.

jonathan-gibbons · 2021-09-21T01:33:09Z

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/resources/doclets.properties

+# 0: path; 1: exception
+doclet.error_setting_snippet_path=\
+  Error setting snippet path {0}: {1}


Add newline at end of file

Although it wasn't there to begin with, adding it won't be as big a deal as reformatting code in other places you suggested earlier.

jonathan-gibbons · 2021-09-21T01:38:03Z

...javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/snippet/MarkupParser.java

+    protected boolean isUnquotedAttrValueTerminator(char ch) {
+        switch (ch) {
+            case ':': // indicates that the instruction relates to the next line
+            case ' ': case '\t':


What about other whitespace? Is that already covered?

Which other whitespace? If you are talking about \r or \n, then the string parsed by MarkupParser cannot have those. MarkupParser gets a single line without a trailing line terminator.

jonathan-gibbons · 2021-09-21T01:39:12Z

...vadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/snippet/ParseException.java

+ *
+ * This exception is only used to capture a user-facing error message.
+ * The message supplier is accepted not to control when to obtain a message,
+ * but to abstract how to obtain in.


typo "in"? maybe "one" or "it"?

Good catch! It should've been "it".